Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tweak: dont enqueue filesystem events #5631

Merged
merged 8 commits into from
Mar 28, 2025
Merged

Conversation

mitchellwrosen
Copy link
Member

@mitchellwrosen mitchellwrosen commented Mar 21, 2025

Overview

Our implementation of filesystem event queueing is simple and straightforward (well the code wasn't, somehow): we essentially just buffer all filesystem events (with a little bit of debounce logic), and processes them one at a time, with user inputs interleaved.

This has some undesirable properties, though. When executing a long-running IO action, or a local http microservice, a bunch of file change events can build up (if editing while the thing is running), which then slowly get worked through upon the IO action completing or web server being torn down.

I had gotten in the bad habit of just holding down Ctrl+C when killing a local http microservice, because of the high likelihood that I had been editing a unison file while it was running and had enqueued a bunch of unwanted file change events.

I changed the implementation to basically just remove the queue. (The diff is big because I also cleaned up a ton of complex stuff). The behavior in this PR is roughly as follows: when the main loop is ready to wait for either an event or a user input, it does so with a freshly-emptied filesystem event queue. I kept the existing debounce logic we already had, which is sort of a two-layered thing: only emit a filesystem event after 50ms elapse without an event (to avoid reacting to the flurry of writes that modern editors typically make), and also don't emit events for files that haven't changed, if the events are within 500ms of each other (I guess this is to save a little CPU?)

Test coverage

I tested this manually at the prompt, it seems to work.

Loose ends

The user experience of editing a large .u file that takes multiple seconds to typecheck could perhaps be further improved.

The trunk behavior is to enqueue one slow typecheck after another, for every edit the user makes, which obviously isn't very nice. This PR's behavior is to basically ignore user edits that are made while a slow type check is in progress. We might instead want to interrupt a typecheck on file change, so that we don't waste time typechecking an old thing, and can just start over with the latest contents.

@mitchellwrosen mitchellwrosen changed the title tweak: dont enqueue events tweak: dont enqueue filesystem events Mar 21, 2025
@mitchellwrosen mitchellwrosen marked this pull request as ready for review March 21, 2025 18:30
@ceedubs
Copy link
Contributor

ceedubs commented Mar 24, 2025

I haven't tried this out to have real-world experience to report, but I'm a bit wary of the behavior of ignoring file updates if a type check is in progress. I'm often working in a scratch file that takes several seconds to type check, and if my saves start getting ignored without noticing, I could see it being a really bewildering experience. And in the worst case I might never push code that I think that I have pushed.

I agree that queuing up additional type checks that don't start until the first one finishes is not useful. But I wonder if getting rid of the queue should hold off until the improvement that you mentioned has been implemented:

We might instead want to interrupt a typecheck on file change, so that we don't waste time typechecking an old thing, and can just start over with the latest contents.

@mitchellwrosen
Copy link
Member Author

@ceedubs Ok, I've made that change. I agree it's better than just leaving it up to the user to realize they last saved their file while a typecheck was in-progress, and they therefore might be looking at an old typecheck result.

Would you mind trying this branch out and report whether it works as you'd like it to?

@ceedubs
Copy link
Contributor

ceedubs commented Mar 24, 2025

Thanks @mitchellwrosen!

Here is an example that I tried. clone @unison/cloud and then edit.namespace. For me each save of my scratch file takes about 40 seconds for ucm to catch up. (Side note: I think that a lot of that time might be spent rendering test results for tests that have already been cached.) Obviously this isn't ideal, but most of the time you probably won't have this much code in your scratch file. BUT a concern for me is whether this might result in basically an endless loop for people who use editors that auto-save files every 5 seconds (or whatever). Will those save again if the file contents haven't changed? And if so will that restart the 40 second file change event? If so, I don't think that there will be any feedback to the user letting them know that they are in an endless loop as opposed to a single file save event. But I'm probably not understanding how this works for people with auto-save editors.

@mitchellwrosen
Copy link
Member Author

@ceedubs A typecheck is only aborted and restarted if the file contents actually change. (Were you observing otherwise?)

Note that the trunk behavior is also not good for a person trying to typecheck a file that takes 40 seconds, with an editor that's auto-saving every 5: a never-ending stream of old typechecking results!

@ceedubs
Copy link
Contributor

ceedubs commented Mar 25, 2025

Were you observing otherwise?

I wasn't sure. As far as I can tell there isn't any feedback that the type checking is restarting. Which is probably fine.

A typecheck is only aborted and restarted if the file contents actually change.

Okay great! 👍

@mitchellwrosen
Copy link
Member Author

I wasn't sure. As far as I can tell there isn't any feedback that the type checking is restarting. Which is probably fine.

Oh, hm, you should see this again, which we output before parsing:

Loading changes detected in ~/scratch.u.

@mitchellwrosen
Copy link
Member Author

Eek, interruption doesn't seem to be working, actually. I guess I need to debug this further.

@mitchellwrosen mitchellwrosen marked this pull request as draft March 25, 2025 14:49
@mitchellwrosen
Copy link
Member Author

Sorry for the thrash, I had an obvious bug, now I feel this PR is doing the right thing. And yeah, if you interrupt one long typecheck with a new one, you will see a repeat of the "Loading changes" message, which indicates we've started over.

@mitchellwrosen mitchellwrosen marked this pull request as ready for review March 25, 2025 14:54
@aryairani
Copy link
Contributor

aryairani commented Mar 26, 2025

One changed behavior I noticed is if you save a blank .u file when ucm is first opened, it doesn't generate any output.

Usually I do this to confirm that the file is "connected" e.g. not the right name in the wrong directory for my ucm session; I'm open to alternative methods though.

Is there an easy fix for that?

@aryairani
Copy link
Contributor

Huh, I can't reproduce now; that was weird.

@aryairani
Copy link
Contributor

Now I can reproduce again. 🤷‍♂️

@aryairani
Copy link
Contributor

I can only sometimes reproduce it.

Could you try to too @mitchellwrosen

@mitchellwrosen
Copy link
Member Author

@aryairani I'm not seeing any difference in behavior between trunk and this branch with respect to saving an empty file. (And furthermore I can't think of anything in the implementation that might have affected that, but I could be forgetting something).

@aryairani
Copy link
Contributor

Can you think of anything that would make the cli tests time out on windows? I'm rerunning them optimistically

@mitchellwrosen
Copy link
Member Author

@aryairani Hmm...

@aryairani
Copy link
Contributor

aryairani commented Mar 27, 2025

I got this with git bisect and the command

stack run unison -- run.file unison-cli-integration\integration-tests\IntegrationTests\print.u print --codebase-create tempcodebase

0abf9e2 is the first bad commit

commit 0abf9e230110efd617a0450cdce04ee5e3103b1f
Author: Mitchell Dalvi Rosen <[email protected]>
Date:   Fri Mar 21 14:10:36 2025 -0400

    rework filesystem event handling so that events aren't queued

 unison-cli/src/Unison/Codebase/Watch.hs   | 226 +++++++++--------------
 unison-cli/src/Unison/CommandLine/Main.hs | 285 ++++++++++++++++--------------
 2 files changed, 234 insertions(+), 277 deletions(-)

@aryairani
Copy link
Contributor

Here's the line we ended up with:

stopListening <- unsafeUnmask (FSNotify.watchDir mgr dir (const True) handler) <|> pure (pure ())

Do you wanna just cherry-pick it into the latest?

Copy link
Contributor

@aryairani aryairani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@aryairani aryairani merged commit 51d6d80 into trunk Mar 28, 2025
32 checks passed
@aryairani aryairani deleted the 25-03-21-dont-enqueue-events branch March 28, 2025 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants